-
Notifications
You must be signed in to change notification settings - Fork 169
Add metadata support to callTool method #289
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add metadata support to callTool method #289
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | ||
|
|
||
| @OptIn(ExperimentalUnsignedTypes::class, ExperimentalSerializationApi::class) | ||
| private fun convertToJsonElement(value: Any?): JsonElement = when (value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense to extract JSON-related code to a separate class. It could be a separate PR with unit tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be moved to separate class
|
Thank you for your feedback, @kpavlov |
|
Thank you, @maeryo |
|
@kpavlov, I just fixed ktlint. 🚀 |
5a73759 to
2b957a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at my comments.
Also, it seems that your changes are breaking the tests
kotlin-sdk-client/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/Client.kt
Outdated
Show resolved
Hide resolved
kotlin-sdk-client/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/Client.kt
Outdated
Show resolved
Hide resolved
kotlin-sdk-client/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/Client.kt
Outdated
Show resolved
Hide resolved
kotlin-sdk-client/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/Client.kt
Show resolved
Hide resolved
|
Sorry, the integration tests broke because of the new field added. I’ll add a fix soon |
|
@devcrocod I've addressed all your comments. Please let me know if any further changes are needed. |
- Add meta parameter support to callTool method with validation - Implement MCP-compliant meta key validation (reserved prefixes, format rules) - Enhance JSON conversion for complex data types with better error handling - Add comprehensive test suite for meta parameter functionality - Improve MockTransport to simulate proper initialization flow - Update API signatures to include meta parameter support
small refactoring
127b270 to
7e087b4
Compare
| Method.Defined.PromptsList, | ||
| Method.Defined.CompletionComplete, | ||
| -> { | ||
| Method.Defined.PromptsGet, Method.Defined.PromptsList, Method.Defined.CompletionComplete -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually better to have each case on a new line, because add/delete is more visible in diff
| val labelPattern = Regex("[a-zA-Z]([a-zA-Z0-9-]*[a-zA-Z0-9])?") | ||
| val namePattern = Regex("[a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])?") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regex patterns should be static constants
| } | ||
|
|
||
| @OptIn(ExperimentalUnsignedTypes::class, ExperimentalSerializationApi::class) | ||
| private fun convertToJsonElement(value: Any?): JsonElement = when (value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be moved to separate class
| "formattedResult" : "11,000", | ||
| "precision" : 3, | ||
| "tags" : [ ] | ||
| "tags" : ["test", "calculator", "integration"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻 something was fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, previously the arrays were serialized as a string, and for some reason they were completely skipped in the process
| client.callTool("test-tool", mapOf("arg" to "value"), validMeta) | ||
| } | ||
|
|
||
| assertTrue(result.isSuccess, "Valid meta keys should not cause exceptions") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assertion for meta content is missing
| } | ||
| } | ||
|
|
||
| class MockTransport : Transport { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MockTransport should be moved to separate file. It's a nice reusable stuff
| } | ||
|
|
||
| class MockTransport : Transport { | ||
| private val _sentMessages = mutableListOf<JSONRPCMessage>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a thread-safe collection
| } | ||
|
|
||
| override fun onMessage(block: suspend (JSONRPCMessage) -> Unit) { | ||
| onMessageBlock = block |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should also capture and expose received messages (thread-safe)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, folks!
I'm fine with merging this PR and making betterments in follow-up PR(s)
|
@kpavlov Thanks for all the feedback. I'll prepare a follow-up PR after this is merged. |
## Changes - Updated `DecimalFormatSymbols` to use period as decimal separator (Locale.US) - Fixed test expectations in `testComplexInputSchemaTool`: - `formattedResult`: `"11,000"` → `"11.000"` - `tags`: Added actual test values `["test", "calculator", "integration"]` - Fixed test expectations in `testComplexNestedSchema`: - `formattedResult`: `"0,00"` → `"0.00"` ## Motivation 1. **Standard Decimal Notation**: Using a comma as a decimal separator could be confusing as it's commonly used for thousands separators in many locales. Standardizing on the period (`.`) follows international conventions and makes the output clearer. 2. **Test Data Correction**: During the Validate PR workflow for #289, discovered that the `tags` array in the `expectedContent` variable of `testComplexInputSchemaTool()` was incorrectly set to an empty array, which didn't match the actual input values being tested. --------- Co-authored-by: MAERYO <[email protected]>
According to https://modelcontextprotocol.io/specification/2025-06-18/basic/index#meta, the _meta parameter should be passable when calling tools. While the CallToolRequest class (in types.kt) currently includes _meta in its definition, SDK users cannot actually input _meta when invoking the callTool method.
This Pull Request adds a metadata parameter to the CallTool method and includes functionality to convert various data formats without loss.
Additionally, to encourage the use of valid Key name formats as defined by the protocol, validation for prefixes has been implemented.